Issue 188: Add delay samples functionality #210

athowes · 2024-07-30T14:06:56Z

Description

This draft PR will close #188.

Checklist

My PR is based on a package issue and I have explicitly linked it.
I have included the target issue or issues in the PR title in the for Issue(s) issue-numbers: PR title
I have read the contribution guidelines.
I have tested my changes locally.
I have added or updated unit tests where necessary.
I have updated the documentation if required.
My code follows the established coding standards.
I have added a news item linked to this PR.
I have reviewed CI checks for this PR and addressed them as far as I am able.

athowes · 2024-07-31T13:23:33Z

To do before merge:

Add issue about helping the user with newdata Help user pass newdata as sensible things (e.g. all strata) #213
Add issue about adding functionality for full custom family implementation things Posterior prediction for lognormal #214
Add issue about summary function built on top of this Create delay_summary function built on predict_delay_parameters #215
Discuss choice of name (maybe different to delay_samples). Could use word draws, could mention extracting. Want to be similar to the summary function

athowes · 2024-07-31T15:01:13Z

I think this is ready for review. I have created follow up issues.

Other thing is that for some reason it's failing a Ubuntu R-CMD-check but in a way that seems not related to the code changes. Any pointers?

seabbs · 2024-07-31T21:01:21Z

The error is here:

Error ('test-unit-postprocess.R:5:3'): add_mean_sd.lognormal_samples works with posterior samples from the latent lognormal model ──
Error: Error: All list elements must have the same length.

You also have a few unrelated to this PR notes and warnings that we should flag and ideally fix

R/postprocess.R

seabbs

This is looking good. Just naming to discuss I think

athowes · 2024-08-01T09:00:26Z

Added issue about fixing possible check notes #217

athowes · 2024-08-01T09:07:22Z

The error is here:

Error ('test-unit-postprocess.R:5:3'): add_mean_sd.lognormal_samples works with posterior samples from the latent lognormal model ──
Error: Error: All list elements must have the same length.

How did you see this? I can just see:

══ Failed tests ════════════════════════════════════════════════════════════════
── Error ('test-unit-postprocess.R:5:3'): add_mean_sd.lognormal_samples works with posterior samples from the latent lognormal model ──
Error in read_cmdstan_csv(files = self$output_files(include_failed = FALSE), variables = variables, sampler_diagnostics = sampler_diagnostics, format = format): Assertion on 'files' failed: File does not exist: '/tmp/RtmpVPgfDE/working_dir/Rtmpkb8cQq/model_ea52f49f447bb88d6fe4ace47415d149-202407312111-1-1098f0.csv'.

athowes · 2024-08-01T10:26:54Z

Should we have functionality for getting a "properly" discretised PMF in this package or somewhere else?

seabbs · 2024-08-01T15:26:39Z

How did you see this? I can just see:

I just looked through the log? What was the fix that made this go away?

seabbs · 2024-08-01T15:28:05Z

Should we have functionality for getting a "properly" discretised PMF in this package or somewhere else?

My vote is somewhere else so that i.e epinowcast can use it and then also use it here but we could dev something here and then port out. Note that epinowcast already has a simulate based approach to this but really the approach taken in EpiAware (numeric integration) is the way to go I think

athowes · 2024-08-01T15:30:59Z

How did you see this? I can just see:

I just looked through the log? What was the fix that made this go away?

I didn't do anything it just went away.

seabbs

This is great. I think there are a few errors in the handling of newdata. I know we are doing the new data stuff separately but I think we need a few minimal checks here to be sure it works as is and avoid broken code going to main

R/postprocess.R

athowes · 2024-08-01T15:42:45Z

Good spot! Agree we should fix these newdata things.

athowes · 2024-08-02T09:51:02Z

I've added tests for this (and indeed they did bring up another code issue! Which is now fixed).

seabbs

Nice. Looks great.

…d predict_dpar alias

* Begin working on delay_samples functionality * Version of delay_samples working with hard coded lognormal * Add documentation and a stop condition if it's not lognormal * Replace use of extract_lognormal_draws with delay_samples * Generalise delay_samples to work for any family * Move delay_samples into fitting-and-postprocessing * Move add_mean_sd into fitting-and-postprocessing * Rename to postprocess.R * Document (and add autoglobals) * Missed this instance of extract_lognormal_draws! * Add option to pass extra things to brms::pp and lint * Fail to update docs! Oops * Add use of validate_newdata * Rename to predict_delay_parameters, add explicit newdata argument, add predict_dpar alias * Correct way to link documentation, and document() * Corrections to predict_delay_parameters and add tests * Move fitting inside tests Former-commit-id: 94bf223791b0a38993981bc689806af5b96a10f8 [formerly bf9c9b6003dcd41b3c788b5d48c652e61644e4c4] Former-commit-id: 946deb6504663aefecfec999f000f4b1002af991

athowes force-pushed the delay-samples branch from ad14b55 to 4919ceb Compare July 31, 2024 11:13

athowes mentioned this pull request Jul 31, 2024

Help user pass newdata as sensible things (e.g. all strata) #213

Closed

athowes requested a review from seabbs July 31, 2024 15:00

athowes marked this pull request as ready for review July 31, 2024 15:00

athowes requested a review from kgostic July 31, 2024 15:01